-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactoring ALP #247
Refactoring ALP #247
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of duplication that could be trimmed down, but as per the guidelines it is not a blocker.
Changes to non aALP parts have some issues though.
python/examples/core/transform.py
Outdated
else [objtype] + other_cls._transform_classes) | ||
return TransformListMetaclass(name, (TransformationList,), {}, | ||
transforms=transforms) | ||
"""Python descriptor dispatching `then` on the `Transform` class as either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has unfortunately too many trivial spacing changes that makes it hard to review more deeply.
Please note the autoformatting setup that we've been using:
https://github.com/nicolasvasilache/nicolas.vasilache.github.io/blob/master/vscode_settings/settings.json#L43
Maybe that will help you reduce the amount of changes so this load-bearing file becomes reviewable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, I was using Black and that file was reformatted like that. I will try your vscode settings. Does it apply the formatting every time you modify a python file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I never heard of YAPF :) Let me try it out !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the config I shared reformats on save
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried YAPF out, but that file still seems to suffer for reformatting. Should I try to add this as an independent change as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, pulling non-experimental changes out of this commit and having a specific reformat CL would be great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please slice this out in a separate PR, address and rebase ?
This way we can land this bigger refactoring.
d926482
to
929c2d2
Compare
@nicolasvasilache I removed the change to |
Great, let's just do the same for configure.py and land this. |
This is a big refactor of our experimental codebase to follow latest developments: - Generate functions through the Python DSL interface - Use the trasnformation infra common in the sandbox - Generalize tuner/mlirc to be program agnostic (although transformations are still GEMM-specific) - Add a python transformation to save the IR - Make `configure.py` compatible with python 3.7
@nicolasvasilache , I think there are no more space-formatting changes around. The only change that affects the main repo is in Please, let me know if there is anything else I need to change/update |
Looks good thanks! |
This is a big refactor of our experimental codebase to follow latest developments:
configure.py
compatible with python 3.7